-
Notifications
You must be signed in to change notification settings - Fork 121
Use bundle versions when adding JUnit to launch #2013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Test Results 771 files ±0 771 suites ±0 1h 11m 57s ⏱️ + 7m 2s For more details on these failures, see this check. Results for commit 2ca58a5. ± Comparison against base commit f588de6. ♻️ This comment has been updated with latest results. |
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
|
@HannesWell could you take a look here as well? |
I'll have a look at it tomorrow. Thanks for working on this! |
HannesWell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work.
I tried to answer at least some of your questions, but I think it needs a bit more general rework to handle versions properly.
Furthermore the class org.eclipse.pde.unittest.junit.launcher.JUnitPluginLaunchConfigurationDelegate, which is more or less a copy needs the similar adjustments too.
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
f1b72a9 to
1515b1f
Compare
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
Unfortunately I don't know enough about the code and available APIs to provide a rework.
I've made change there too now. |
|
@HannesWell can you continue reviewing this? It would be nice to continue working on JUnit 6, which is blocked by the issue here. |
315b26e to
8a1fa96
Compare
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/PluginBlock.java
Outdated
Show resolved
Hide resolved
HannesWell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell can you continue reviewing this? It would be nice to continue working on JUnit 6, which is blocked by the issue here.
Yes, I'll continue tomorrow with an in-depth review. Sorry for the delay.
For now I just have a few style remarks.
Furthermore it looks like you added more changes to the version-bump commit pushed by the bot. I suggest you squash your changes into one commit and remove the version bump so that the bot can do it's thing again after you (force) pushed the update.
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/PluginBlock.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
b6c7fa7 to
b1232ad
Compare
|
Lets wait for test results, if everything is fine we should be able to merge. |
and define runtime requirements in the MANIFEST.MF with proper version-ranges instead of in the pom.xml with unbound versions. Required for - eclipse-pde#2013
Yes, lets extract that: |
Extracted from and required by: - eclipse-pde#2013
and define runtime requirements in the MANIFEST.MF with proper version-ranges instead of in the pom.xml with unbound versions. Required for - #2013
Extracted from and required by: - #2013
|
With the version bump commit gone, local build fails for me with: I mean this commit: Other than this, there is no hang and the tests that were hanging pass: |
|
@trancexpress I'm currently prepare one more PR and then this error will be gone. |
|
@trancexpress the pr (#2044) is merged now but GitHub is not smart enough to automatically rebase, can you do the rebase by:
That the hopefully should leave us with a PR that has a nice diff and mostly only |
and remove unnecessary separate attempts to add junit-launcher and jupiter.engine bundles to the runtime, intending to support @RunWith(JUnitPlatform.class), because these bundles always are already added to a runtime. This change adjusts JUnitLaunchConfigurationDelegate, to consider bundle versions when adding required JUnit bundles. E.g. when adding JUnit bundles for a JUnit 5 test, JUnit 5 versions are added. This avoids conflicts when both JUnit 5 and JUnit 6 bundles are in the platform, since they share symbolic names. Furthermore handle multiple versions of a bundle when searching them in the host. Fixes: eclipse-pde#2006 Co-authored-by: Hannes Wellmann <[email protected]> Co-authored-by: Ed Merks <[email protected]>
|
Great, many thanks @laeubi ! Lets see the test results, if all is well lets merge. I'll do checks with the aggregator repository in parallel, but any left-over we can continue in a separate PR. It will be way easier to test with this merged. |
|
The fail on Windows is: That does look related, though why only on Windows I have no idea. Also I have no Windows machine so I can't check the fail. |
|
I'm rerunning the Windows job. Probably just a glitch. Goodness knows we have plenty of those. |
|
FYI, I do have a Windows machine so I will rerun the local Tycho build with this PR to test it. |
|
It works for me locally: |
|
Great, thank you as well @merks , @HannesWell ! |
|
It's great that this has finally landed.
Just for you infromation, in case you encounter this again. The error says that the content of the project |
I see, thank you for letting me know! |
and define runtime requirements in the MANIFEST.MF with proper version-ranges instead of in the pom.xml with unbound versions. Required for - eclipse-pde#2013
Extracted from and required by: - eclipse-pde#2013


This change adjusts JUnitLaunchConfigurationDelegate, to use bundle versions when adding required JUnit bundles.
E.g. when adding JUnit bundles for a JUnit 5 test, JUnit 5 versions are added.
This avoids conflicts when both JUnit 5 and JUnit 6 bundles are in the platform, since they share symbolic names.
Fixes: #2006